feat(compact): reactive compaction with direct input support - #726
feat(compact): reactive compaction with direct input support#726crstrn13 wants to merge 3 commits into
Conversation
praxis-bot
left a comment
There was a problem hiding this comment.
PR Review
Summary: Completes five compaction features from #30: previous_usage token estimation fast-path, configurable summary_prefix, hiding compaction items from input_items, persisting compaction responses, and an explicit POST /v1/responses/compact endpoint.
Overall: Solid implementation with good integration tests covering both the rehydrated and direct-input paths. The summary prefix plumbing is clean and consistent across both translation paths. A few issues below around blocking in async context, stale docstrings, and missing unit test coverage for the new endpoint.
| Severity | Count |
|---|---|
| Large | 3 |
| Medium | 3 |
Findings without inline placement
[Large] Stale docstrings — The struct-level doc comment on CompactFilter (~line 96 of mod.rs) says compaction "only applies to multi-turn requests where openai_responses_rehydrate has loaded stored conversation history". The generated docs page (docs/filters/openai_responses_compact.md, Configuration Notes section) says the same. Both are now incorrect — this PR adds a direct-input compaction path that runs without rehydration. Update both to match the module-level doc comment (lines 14-20), which was correctly updated.
[Large] Missing unit tests for explicit compact endpoint — The new pure/mostly-pure functions parse_compact_request_body, extract_stored_messages, is_explicit_compact_request, and ensure_compactable_state have no unit tests. Per project convention each testable function should have coverage. Suggested cases:
parse_compact_request_body: empty body, invalid JSON, missingresponse_id, valid request with optional fieldsextract_stored_messages: empty array, non-array messages, valid arrayis_explicit_compact_request: POST to correct path, GET to correct path, POST to wrong pathensure_compactable_state: noResponsesState, rehydrated state, non-rehydrated with compaction config, non-rehydrated without compaction config
| req: &ExplicitCompactRequest, | ||
| ) -> Result<ResponseRecord, FilterAction> { | ||
| let handle = tokio::runtime::Handle::current(); | ||
| match tokio::task::block_in_place(|| handle.block_on(store.get_response(tenant_id, &req.response_id))) { |
There was a problem hiding this comment.
[Large] fetch_response_blocking uses block_in_place + block_on to call the async store.get_response, but its only caller (do_explicit_compact) is itself async. The same pattern appears in build_and_persist_compaction (the store.upsert_response call around line 607). Both block a tokio worker thread unnecessarily during potentially slow I/O (e.g., a PostgreSQL round-trip).
Make both functions async and .await the store calls directly. The persist_compaction_response function uses the same block_in_place pattern but its caller (apply_compaction) is not async, so block_in_place is correct there — no change needed for that one.
| return Ok(FilterAction::Continue); | ||
| } | ||
| if is_explicit_compact_request(ctx) { | ||
| return self.handle_explicit_compact(ctx, _body).await; |
There was a problem hiding this comment.
[Medium] The on_request_body parameter is named _body (underscore prefix indicating unused), but it is now passed to handle_explicit_compact on this line. Rename the parameter from _body to body since it is actively used in the explicit compact path.
| .get("response_id") | ||
| .and_then(Value::as_str) | ||
| .ok_or_else(|| reject_compact(400, "invalid_request_error", "missing required field: response_id"))? | ||
| .to_owned(); |
There was a problem hiding this comment.
[Medium] response_id is not validated for emptiness. An empty string ("response_id": "") passes this check and produces a confusing 404 "response not found" instead of a clear 400 error. Add an empty check:
if response_id.is_empty() {
return Err(reject_compact(400, "invalid_request_error", "response_id cannot be empty"));
}| get_token_count(&overhead_text, tiktoken_encoding).unwrap_or(0) | ||
| }; | ||
| let conversation_text = build_conversation_text(&state.messages); | ||
| let token_count = get_token_count(&conversation_text, tiktoken_encoding)?; |
There was a problem hiding this comment.
[Medium] The tiktoken fallback path here no longer accounts for instructions and tool-definition overhead — the old build_context_overhead_text was removed. When previous_usage.total_tokens is available it naturally includes all tokens, but the tiktoken path now only counts messages. For the direct-input path (which always hits tiktoken since there is no previous_usage), large system prompts and tool definitions will not count toward the threshold.
This may be intentional, but it is a behavioral change from the prior code. Either restore overhead counting for the tiktoken branch, or add a targeted test that documents the difference and asserts the intended behavior.
43ac0b3 to
b0046dd
Compare
praxis-bot
left a comment
There was a problem hiding this comment.
Re-review (Aug 18 commits)
Previous review had 6 findings. The new commits addressed 4 fully (async conversion of fetch_response/build_and_persist_compaction, _body rename, response_id emptiness validation, tiktoken overhead restoration). One partially addressed (unit tests added for parse_compact_request_body and extract_stored_messages, but is_explicit_compact_request and ensure_compactable_state still lack coverage). One unaddressed (stale docstrings).
One new finding below from the rewritten explicit compact endpoint.
| Severity | Count |
|---|---|
| Medium | 1 |
aslakknutsen
left a comment
There was a problem hiding this comment.
Outside the PR diff, but
ai/apis/src/openai/responses/store/filter.rs
Line 464 in bc1e5bd
"ResponseStoreFilter::should_init_store_for_request() returns false for POST /v1/responses/compact because the path is not is_responses_create and the classifier does not set has_previous_response_id for sub-resource routes. The store Arc is therefore never registered into ResponseStoreRegistry before CompactFilter::resolve_store_and_tenant() runs. On a freshly built pipeline (or after reload), the first explicit compact call fails with "response store not available" even when the backend is healthy. Extend should_init_store_for_request (or equivalent) to initialize the store for POST /v1/responses/compact, mirroring how rehydrate paths are handled."
| // ----------------------------------------------------------------------------- | ||
|
|
||
| /// Returns `true` when compaction should proceed. | ||
| fn ensure_compactable_state(ctx: &HttpFilterContext<'_>) -> bool { |
| // ----------------------------------------------------------------------------- | ||
|
|
||
| #[test] | ||
| fn compact_passthrough() { |
There was a problem hiding this comment.
"AGENTS.md requires functional integration coverage for new capabilities in example configs."
No tests POST to /v1/responses/compact.
praxis-bot
left a comment
There was a problem hiding this comment.
PR Review (re-review)
The Aug 20 commits fixed the response_object finding from the previous re-review (now includes model, created_at, and output).
One new finding from the direct-input compaction path.
| Severity | Count |
|---|---|
| Medium | 1 |
| state.persisted_messages = new_messages.clone(); | ||
| let direct_input = !state.history_rehydrated; | ||
| let new_messages = if direct_input { | ||
| vec![compaction_item] |
There was a problem hiding this comment.
[Medium] In the direct-input reactive path, replace_messages drops the entire conversation — including the current user question — and replaces it with [compaction_item]. The compaction item translates to an assistant-role summary, so the backend receives a request with no user message.
Most Chat Completions backends reject requests without a user message (e.g. OpenAI returns "Messages must contain at least one user message"). Even Responses API backends would have no explicit question to answer — the model would generate a generic continuation of the summary rather than responding to the user's actual question.
The integration test compact_direct_input_compacts_full_conversation asserts this as correct (input.len() == 1), but it passes only because the mock backend returns a canned response regardless of input.
Suggested: either (a) skip reactive compaction in the direct-input path — the explicit POST /v1/responses/compact endpoint already covers the non-rehydrated use case — or (b) heuristically preserve the trailing user message(s) from state.input as the "current turn" so the backend has a question to answer.
There was a problem hiding this comment.
I decided to take path a instead due to simplicity. If a user wants compact without rehydration, it can use the explicit POST /v1/responses/compact
9d6b735 to
3a48a2c
Compare
praxis-bot
left a comment
There was a problem hiding this comment.
Re-review (Aug 24 commits)
The two new commits fix the summarization scope (exclude current input from summarization text in the rehydrated path), add is_compactable unit tests, add an explicit compact endpoint integration test, fix the stale docstrings on CompactFilter and the docs page, and add store initialization for the compact endpoint.
Previous findings addressed:
- Stale docstrings: fixed (struct doc and docs page updated)
- Missing
is_compactable/ensure_compactable_statetests: fixed (extractedis_compactableand added 5 tests)
One new finding below.
| Severity | Count |
|---|---|
| Medium | 1 |
|
@crstrn13 please rebase |
b9908e8 to
e849dfe
Compare
|
Missing Signed-off-by: e849dfe. All commits require sign-off (via |
e849dfe to
d2de464
Compare
…roxy#30) Signed-off-by: Alexander Cristurean <acristur@redhat.com>
f6dc948 to
359559a
Compare
leseb
left a comment
There was a problem hiding this comment.
P1 — Explicit compact rejects conforming requests. parse_compact_request_body requires nonstandard response_id. The checked-in API contract requires model and supports input/previous_response_id. Standard {model,input} requests therefore return 400.
P1 — Response violates CompactResource. The response builder returns object: "response" without usage; the contract requires object: "response.compaction" and usage.
P2 — Fail-open becomes fail-closed. Explicit compaction converts the Ok(None) produced by on_failure: open into a hard-coded 502.
P2 — Reactive compaction creates unreachable rows. apply_compaction generates and persists a response ID, then discards it without exposing or linking it. Every compaction leaves an orphan record despite the normal response subsequently persisting the compacted history.
Summary
Completes the remaining work items from #30:
previous_usagefor token estimation —should_compact()checks the storedusage.total_tokensfrom the rehydrated response before falling back to local tiktoken countingsummary_prefixconfig option (default:[Previous conversation summary]) carried through to compaction items and both translation pathsinput_itemsAPI —normalize_input_items()filters out{"type": "compaction"}items so clients never see internal statePOST /v1/responses/{id}/compacttriggers compaction on a previously stored responseTest plan
cargo test -p praxis-ai-apis— new unit tests for all 5 itemscargo test -p praxis-ai-filters— no regressions in filter testsmake lintpasses